Repository navigation
Conversation
A card offload knows its source: everything under one root, and the only
question is whether it arrived intact. This is the other job, the one that
turns up when a cut comes back from an editor. Which files does this timeline
need, and are they all here?
offloader resolve --timeline "01 Chairs Row V6.xml" --search-root E:\ChairsDoc
offloader offload --timeline "..." --search-root E:\ChairsDoc --dest E:\...
Reading timelines is OpenTimelineIO's job and nothing else's, behind a new
[timeline] extra. The adapters are not in OTIO's core distribution, so a bare
install reads .otio and none of the formats an NLE exports; the extra names
otio-fcp-adapter explicitly.
OTIO is trusted with media references and not with structure. Measured against
a Premiere-exported FCP 7 XML of 913 clipitems, it recovered all 334 unique
references with none missed and none invented, including the clipitems whose
<file> is an id reference carrying no path of its own. On the same file it
reported 29.97 fps and 12 audio tracks for a sequence that declares 24 and 23.
So no rate, duration or track count from the adapter reaches a caller.
The resolver is the part worth having. Six statuses, and the order of the
tests is the design: a generated reference stops at once; anything under a
search root with that basename wins whatever path the timeline asked for; a
camera original may be satisfied by a proxy if the frame counts agree; only
then is a readable file a gap.
It refuses to choose between two files that share a name. On the conform this
was written against, 20 basenames had more than one copy under the search
root. Seven were byte-identical and harmless. Thirteen were different files,
eleven of them MISSING MEDIA stand-in slates from an earlier pass sitting
beside the real archival footage that arrived later. Relinking by filename
picks one at random, and when it picks a slate the clip reports as online.
The closest-path candidate is offered as an explicitly labelled guess; it was
right all thirteen times, which is a reason to show it and not a reason to act
on it unsupervised.
Byte-identical duplicates resolve normally and are not copied again. On that
job that avoided 1.4 GB of pointless transfer and, more to the point, avoided
manufacturing 32 filename collisions on a drive that had none.
A proxy standing in for a camera original is checked frame for frame. A proxy
one frame short moves every edit point after it. Where the count cannot be
established the note says unverified rather than implying agreement; where it
is established and disagrees, nothing is substituted.
Engine: OffloadOptions.selection takes an explicit file set in place of
scanning one root, so a job can span volumes. Each file carries its own
destination-relative path and the engine infers no layout.
The safety rule had to change, and narrowing it was the point.
assert_safe_destinations refuses a destination inside the source, which is
right for a card because the source root is the card. It is wrong for a
selection, where the search roots are a library being read and collecting a
cut's gaps into a folder on that same drive is the ordinary case. A selection
is held instead to the property that actually matters and is checkable
exactly: no file being read may sit at or beneath somewhere being written. A
destination-relative path that is absolute, or climbs out with .., is refused
before the job starts rather than partway through it.
33 tests. The eight that exercise the selection path fail without the engine
change. Verified end to end against two real timelines: 334 references
reconciled file by file against an independent hand-built list, zero missed
and zero extra, 15 proxy substitutions all frame-verified.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
CI on macOS and Linux caught this; Windows passed. After the scheme and any localhost authority, what is left always begins with a slash: /E:/Media/a.mov, or /Volumes/Edit/a.mov. That slash belongs to a POSIX path and must stay, but on a Windows path it sits in front of the drive letter and must go. Stripping it unconditionally turned /Volumes/Edit/a.mov into the relative path Volumes/Edit/a.mov. The failure is close to invisible, which is why it earns a long comment. A relative path still resolves by basename, so a resolve run looks entirely correct; what breaks is every test of whether a file is where the timeline says it is, so a reachable file reads as missing rather than as a gap. An editor cutting on a Mac addresses every clip that way, so this is the common case, not an edge. Tested as a pure function over both platforms' forms, including UNC and percent-escapes, so the coverage no longer depends on which runner it lands on. Re-checked against the two production timelines: 327 references, zero status changes.
JobControl has existed since the desktop app needed transport buttons, and run() checks it once per 8 MiB chunk, but cli.py never passed one. So a job started in a terminal could only be killed: the capability was there and nothing reached it. --control-file PATH wires a FileControl to a file, and "offloader control PATH --pause|--resume|--cancel" drives it from anywhere. A file rather than a signal or a keypress because Windows has almost no signal support beyond SIGINT, a 1.9 TB offload is started detached or over ssh with no console to type into, and a file needs no port and no daemon, survives the terminal closing, and can be read to see what a job is doing. The failure modes are the design. The file holds one word; anything else - empty, garbled, half-written, or momentarily unreadable because another process has it open - is no opinion, and leaves the job in whatever state it is already in. Inferring cancel from a damaged control file would let a stray byte stop an offload that is nine hours in, and a small text file is exactly what a sync client or an editor rewrites in two steps. The control command writes through a staging file and renames it into place, so a job polling between chunks cannot read a truncated instruction. Starting a job claims the path by writing run to it first, or a stale pause from a previous job would stop the next one before it copied a byte with nothing on screen to say why. A paused job keeps polling, so it can still be cancelled; JobControl alone would block inside Event.wait() and never see it. 11 tests. The cancel tests drive the transition from the progress callback rather than after a sleep, because a dozen 400 KB files finish faster than any wall-clock guess; the suite was run five times over to confirm it. One of them also covers the handler never being registered in main(), which argparse hides: --help works from the parser alone, so the command looks fine until it is actually run, which is how that shipped broken for an hour here.
owenpkent
left a comment
owenpkent
left a comment
There was a problem hiding this comment.
Reviewed timeline parsing/resolution, selected-file copying, source/destination validation, control-file handling, CLI verdicts, and their tests. Two malformed-control-file defects are detailed inline. Additional timeline completeness/missing-media findings and synthetic reproductions are supplied privately under SECURITY.md.
The current head also has a merge-blocking CI problem: Ubuntu 3.10, Ubuntu 3.13, Ubuntu with ffmpeg, and macOS fail the new pause/cancel integration tests. For example, the macOS job fails at test_control.py:215 and :264. The small fixtures can finish before FileControl's minimum 50 ms polling interval expires, even when the test requests poll=0.0. Make the tests deterministic with an injected clock/poll mechanism or a synchronized blocking reader, then require the affected platform jobs to pass. Windows passing by timing does not establish the intended behavior.
Local validation here used focused synthetic scripts against this exact head and read-only inspection of the existing CI failure logs. A new full local suite was not run.
Two ways a damaged control file could stop a running offload, both of them the opposite of what the file's own docstring promises. read() compared word[0] after splitting, so "cancel pending upload" - the kind of thing an editor or a sync client leaves behind - read as a cancellation. It now compares the whole stripped, case-folded value against the three states, so anything with a token after it is no opinion. read_text(encoding="utf-8") raises UnicodeDecodeError, which is not an OSError and escaped the handler beside it. A control file holding two bytes of FF FE aborted the checkpoint instead of being read as damage. Caught now alongside the read errors, leaving the job in whatever state it was in. The pause/cancel integration tests were failing on Ubuntu 3.10, Ubuntu 3.13, Ubuntu with ffmpeg and macOS while passing on Windows, which is timing rather than behaviour. poll has a deliberate 50 ms floor, so poll=0.0 did not mean what those tests assumed: sync() skips the read until the interval elapses, and ten 400 KB files can finish between two reads without ever seeing the word written midway. FileControl now takes an injectable clock, defaulting to time.monotonic, so a test can decide the limiter has elapsed instead of hoping the machine is slow enough. Production behaviour is unchanged and the floor stays. The pause tests also write their instruction from the progress callback rather than from the main thread after start, the way the cancel test already did, and write it once: several events fire per file, and rewriting pause after the resume put the job straight back to sleep. Five regressions: a trailing token after each of the three valid words, invalid UTF-8, and a paused job held through both kinds of damage without cancelling. The control suite runs in 2.1s rather than 22.5s, which is the wall-clock the races were costing, and was run five times over to confirm it.
owenpkent
left a comment
owenpkent
left a comment
There was a problem hiding this comment.
Follow-up review of bb99e8f, concentrating on control-file parsing and the previously timing-dependent integration tests.
The parser now accepts the complete control word, handles invalid UTF-8 as damaged content, and preserves the previous state when a damaged file supplies no valid instruction. The integration tests drive the polling clock and request a pause from the progress callback, avoiding dependence on whether a tiny transfer takes longer than the 50 ms polling floor.
Validation: 9 focused tests passed on Linux, including pause/resume, cancellation while paused, trailing-token rejection, invalid UTF-8, and preservation of a paused state after damage. Current-head CI also reports success. The prior control-file findings are addressed. This follow-up does not resolve the separate timeline-completeness concerns referenced in the earlier review.
owenpkent
left a comment
owenpkent
left a comment
There was a problem hiding this comment.
One new P1 data-safety finding was confirmed with disposable synthetic data. Its trigger, affected call path, observed output and regression direction are recorded in a private local review note under SECURITY.md. This public review intentionally omits the reproduction.
Reviewed timeline parsing/resolution, selected-file copying, CLI control wiring, engine boundaries and the added tests. The earlier complete-control-word and invalid-UTF-8 findings are addressed, and the integration tests now control timing explicitly. Separate previously private timeline-completeness concerns are not resolved by those parser corrections.
Validation: all 62 timeline/control tests passed in the exact-head detached worktree. A separate synthetic CLI experiment reproduced the new P1 outside that coverage. Current-head CI reports successful supported-platform, minimum-Python, ffmpeg, soak and distribution jobs. The diff check passes.
Limits: no original footage or user configuration was used; no real editing-project corpus, removable-media fault or full local suite was exercised. This is not an approval or a clean data-safety verdict.
Pinned scope: declared base 8efa1df27eddb808aa038a48091406456fd45cfa, merge base 8efa1df27eddb808aa038a48091406456fd45cfa, head 67fac2fc137a48dae517ca6e194ba3e2e9498862. Existing discussion and current-head checks were reviewed; no threads were resolved and no formal approval or merge is being issued.
# Conflicts: # tests/test_control.py
…romotion target FileControl.claim() wrote the run state to whatever path --control-file named, before engine.run() had validated anything. A path that aliased a source clip truncated it to five bytes before it was hashed; the engine then copied and verified the replacement text, reported Verified, and exited 0. The same write would destroy an existing good destination file. claim() now creates a missing file exclusively and overwrites an existing one only when it is empty or holds a control word. A new guard, run by both the engine and the CLI before anything is written, refuses a control path inside the source, equal to a selected media file or a destination root, or equal to any planned final or partial destination path. Symlinks are followed on every side. The CLI reports the refusal and exits 3 without copying.
…olved A timeline offload could print "nothing to copy: every reference is already here", end Verified and exit 0 while references were missing, ambiguous, offline or unsupported, and the reports listed only what was copied. One same-named file under the search root satisfied several distinct references, so the second recording in the docs' own example was never copied; a readable original was declared present at a different-size stand-in; offline media and image sequences were lumped into "generated"; a control-file cancel exited 0; an unverifiable proxy stood in for the original without a word in the terminal; and relative paths resolved against the working directory rather than the timeline. The job now carries its unresolved references: the final status is Incomplete, the exit code is 1, the terminal lists them, and the CSV, HTML and PDF reports gain an unresolved section. Distinct references sharing a name must resolve to distinct files or stay ambiguous, a readable original is compared with its candidate by size then hash, offline and unsupported references keep their names and fail the verdict, a proxy with no frame count does not stand in, a cancel exits 130 with the not-attempted count, and relative paths anchor to the timeline's folder while non-file URLs are reported as external.
# Conflicts: # CHANGELOG.md # CONTRIBUTING.md # README.md # docs/data-safety.md # src/offloader/cli.py # src/offloader/engine.py # src/offloader/models.py # tests/test_control.py
# Conflicts: # CHANGELOG.md # CONTRIBUTING.md # README.md
1 participant
Stacked on #5, so it targets
proxies-first. Review that one first.A card offload knows its source: everything under one root, and the only
question is whether it arrived intact. This is the other job, the one that
turns up when a cut comes back from an editor.
Which files does this timeline need, and are they all here?
resolvecopies nothing and answers the question.offloadcopies thedifference, with the same verified copy, checksums and reports as a card.
What OpenTimelineIO is trusted with
Reading. Nothing else, and the boundary is measured rather than assumed.
Against a Premiere-exported FCP 7 XML of 913 clipitems, the
fcp_xmladapterrecovered all 334 unique media references, none missed and none invented —
including the clipitems whose
<file>is an id reference carrying no path ofits own. A sweep that reads only
pathurlfinds a source's first use andsilently misses every later one, which is how a file gets left out of a
package.
Its reading of structure, on the same file, is wrong:
So nothing here reads a rate, duration, track or timecode from the adapter, and
none reaches a caller. If that ever changes it needs its own measurement first.
The adapters are also not in OTIO's core distribution — a bare
pip install opentimelineioreads.otioand none of the formats an NLEactually exports — so the
[timeline]extra namesotio-fcp-adapterexplicitly, and
docs/timeline.mdcarries the table for FCPXML, EDL and AAF.Why
ambiguousrefuses instead of choosingThis is the part that earns the feature.
On the conform it was written against, 20 basenames had more than one copy
under the search root. Seven were byte-identical, which is harmless. The
other thirteen were different files sharing a name:
sitting beside the real archival footage that arrived weeks later.
(
f2/260223_009.WAVandf3/260223_009.WAV) — different recordings thathappen to be named alike.
Relinking by filename picks one at random. When it picks a slate, the clip
reports as online, and the cut looks fine until someone watches it.
So the resolver reports every candidate and copies nothing. It offers the
candidate whose trailing path components match the timeline's for longest, as
an explicitly labelled guess. That heuristic was right all thirteen times,
which is a good reason to show it and not a good enough reason to act on it
unsupervised.
Byte-identical duplicates are not ambiguous: they resolve to one copy and are
not offloaded again. On that job this avoided 1.4 GB of pointless transfer and,
more to the point, avoided manufacturing 32 filename collisions on a drive that
had none.
Proxies may stand in, but only frame for frame
A timeline referencing 55 GB of BRAW usually references it because that is what
the camera wrote, not because the cut needs it — the editor cut proxies, and
the proxies are already on the drive.
substitutedmeans a stem-matched proxy was found and the frame countsagree. Relink a clip to a file one frame shorter and every edit point after
it moves, quietly. The count comes from
probe.py, which reads BRAW out of thecontainer because ffprobe cannot open the format at all. Where a count cannot
be established the note says
unverifiedrather than implying agreement; whereit is established and disagrees, nothing is substituted and the reference is
reported
ambiguous.Engine: an explicit file set
OffloadOptions.selectiontakes(source, relative, root)in place ofscanning one root, so a job can span volumes. Each file carries its own
destination-relative path; the engine infers no layout.
The safety rule had to change, and narrowing it was the point.
assert_safe_destinationsrefuses a destination inside the source. That isright for a card, where the source root is the card and writing into it is
how you lose it. It is the wrong question for a selection, where the search
roots are a library being read and collecting a cut's gaps into a folder on
that same drive is the ordinary case, not a mistake:
A selection is held instead to the property that actually matters and is
checkable exactly: no file being read may sit at or beneath somewhere being
written. That admits the case above and still refuses the one that destroys
data. A destination-relative path that is absolute, or that climbs out with
.., is refused before the job starts rather than partway through it. Twoselected files landing on one path are refused by the existing collision check,
which matters more here than for a card: drawing from several volumes at once
makes a name clash the normal case rather than the unlucky one.
Testing
33 new tests, 630 in the suite, ruff clean.
Per CONTRIBUTING, the eight tests that exercise the selection path were checked
to fail without the engine change —
engine.pystashed, suite re-run, alleight failed, restored.
The resolver tests need no OpenTimelineIO: they build
Referenceobjectsdirectly, because what is under test is the decision, not the parse. Only the
reader tests need the adapter, and they skip without it.
Verified end to end against two real timelines: 334 references reconciled file
by file against an independently hand-built list — zero missed, zero extra —
15 proxy substitutions all frame-verified, and the ambiguity list reproduced
exactly. Running it also corrected two counts in the hand pass it was checked
against, which is the argument for having it.
Not in scope
between two files that share a name has a person's judgement in it, and a
tool that made that choice silently would be worse than no tool.
presentandsubstitutedarenot offloaded, because copying them would put a second file of that name on
the drive and create the ambiguity this module exists to detect.
Review follow-ups (2026-10-12)
--control-fileclaimed its path before the engine validated anything, so a path that aliased a source clip truncated it to five bytes and the job verified the replacement. The claim now creates exclusively or overwrites only a control word, and a guard run by both the engine and the CLI refuses a control path inside the source, equal to selected media or a destination root, or equal to any planned destination path.